Faster IFeatureCollection.Get<TFeature> #2290

benaadams · 2018-02-03T02:25:30Z

Go via Generic rather than wrapping with (TFeature)Get(typeof(TFeature)) as it scales better

pre

                 Method |     Mean |     Error |    StdDev |         Op/s | Allocated |
----------------------- |---------:|----------:|----------:|-------------:|----------:|
     GetViaTypeOf_First | 12.56 ns | 0.1568 ns | 0.2347 ns | 79,608,881.2 |       0 B |
    GetViaGeneric_First | 22.93 ns | 0.1275 ns | 0.1908 ns | 43,613,651.3 |       0 B |
      GetViaTypeOf_Last | 75.97 ns | 0.1121 ns | 0.1678 ns | 13,163,676.0 |       0 B |
     GetViaGeneric_Last | 85.30 ns | 0.2586 ns | 0.3870 ns | 11,723,594.4 |       0 B |
    GetViaTypeOf_Custom | 77.44 ns | 0.2239 ns | 0.3351 ns | 12,912,708.4 |       0 B |
   GetViaGeneric_Custom | 86.67 ns | 0.3066 ns | 0.4590 ns | 11,538,511.2 |       0 B |
  GetViaTypeOf_NotFound | 77.56 ns | 0.2899 ns | 0.4339 ns | 12,893,115.2 |       0 B |
 GetViaGeneric_NotFound | 86.23 ns | 0.2085 ns | 0.3121 ns | 11,597,065.2 |       0 B |

post

                 Method |     Mean |     Error |    StdDev |         Op/s | Allocated |
----------------------- |---------:|----------:|----------:|-------------:|----------:|
     GetViaTypeOf_First | 11.85 ns | 0.0565 ns | 0.0846 ns | 84,418,379.7 |       0 B |
    GetViaGeneric_First | 21.11 ns | 0.4077 ns | 0.6102 ns | 47,376,257.7 |       0 B |
      GetViaTypeOf_Last | 74.42 ns | 0.3818 ns | 0.5714 ns | 13,437,263.4 |       0 B |
     GetViaGeneric_Last | 40.41 ns | 4.6554 ns | 6.9680 ns | 24,748,132.8 |       0 B |
    GetViaTypeOf_Custom | 76.88 ns | 0.3606 ns | 0.5397 ns | 13,006,456.8 |       0 B |
   GetViaGeneric_Custom | 41.89 ns | 0.1967 ns | 0.2943 ns | 23,872,788.5 |       0 B |
  GetViaTypeOf_NotFound | 77.13 ns | 0.1864 ns | 0.2790 ns | 12,965,485.2 |       0 B |
 GetViaGeneric_NotFound | 41.31 ns | 0.1276 ns | 0.1910 ns | 24,204,614.8 |       0 B |

#2012 rebased

Resolves #2303

BrennanConroy · 2018-02-03T04:11:20Z

@pakrym

benaadams · 2018-02-10T22:04:45Z

PR also shaves a significant amount off OnReset()

52 secs of plaintext
Pre

Post

benaadams · 2018-02-10T22:48:21Z

CS0103: The name 'PipeFactory' does not exist in the current context

C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\HttpProtocolFeatureCollectionTests.cs(34,24): error CS0103: The name 'PipeFactory' does not exist in the current context [C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\Kestrel.Core.Tests.csproj]
C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\HttpProtocolFeatureCollectionTests.cs(34,24): error CS0103: The name 'PipeFactory' does not exist in the current context [C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\Kestrel.Core.Tests.csproj]
C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\HttpProtocolFeatureCollectionTests.cs(34,24): error CS0103: The name 'PipeFactory' does not exist in the current context [C:\projects\kestrelhttpserver\test\Kestrel.Core.Tests\Kestrel.Core.Tests.csproj]
    0 Warning(s)
    3 Error(s)
Time Elapsed 00:01:51.28

davidfowl · 2018-02-10T23:06:35Z

I broke your change? 😄

benaadams · 2018-02-10T23:22:17Z

I broke your change?

Was trying to work out how it was possible, then realized it was my new test breaking 😄

benaadams · 2018-02-11T00:09:04Z

Set and Get branches

Pre

Post

benaadams · 2018-02-11T00:54:29Z

Full Get usage

Pre

Post

benaadams · 2018-02-11T01:30:21Z

OSX UnitTests/netcoreapp2.0

Failed   NonListenerPipeConnectionsAreLoggedAndIgnored
Error Message:
 System.IO.IOException : Unable to transfer data on the transport connection: Connection reset by peer.
---- System.Net.Sockets.SocketException : Connection reset by peer
Stack Trace:
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.IO.StreamReader.<ReadBufferAsync>d__65.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at System.IO.StreamReader.<ReadToEndAsyncInternal>d__61.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Testing.HttpClientSlim.<ReadResponse>d__4.MoveNext() in C:\b\w\33bdfc1cae7b2a38\modules\Testing\src\Microsoft.AspNetCore.Testing\HttpClientSlim.cs:line 66
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Testing.HttpClientSlim.<GetStringAsync>d__1.MoveNext() in C:\b\w\33bdfc1cae7b2a38\modules\Testing\src\Microsoft.AspNetCore.Testing\HttpClientSlim.cs:line 35
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests.ListenerPrimaryTests.<AssertResponseEventually>d__3.MoveNext() in /_/test/Kestrel.Transport.Libuv.Tests/ListenerPrimaryTests.cs:line 281
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests.ListenerPrimaryTests.<NonListenerPipeConnectionsAreLoggedAndIgnored>d__1.MoveNext() in /_/test/Kestrel.Transport.Libuv.Tests/ListenerPrimaryTests.cs:line 139
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
----- Inner Stack Trace -----

OSX UnitTests/netcoreapp2.1 pass

halter73

Any idea why GetViaGeneric_First is still twice as fast as GetViaGeneric_Last? Shouldn't the JIT remove all the extra branches so it optimizes to return (ReifiedType)_specificFeature?

halter73 · 2018-02-16T23:36:43Z

src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs

-        }
-
-        TFeature IFeatureCollection.Get<TFeature>()
+        protected void ResetIHttpUpgradeFeature()


Nit: missing newline

Theoretically, isn't this exactly what Set<IHttpUpgradeFeature>(this) should optimize to?

Comment below

Added newline

benaadams · 2018-02-17T02:27:45Z

Any idea why GetViaGeneric_First is still twice as fast as GetViaGeneric_Last? Shouldn't the JIT remove all the extra branches so it optimizes to return (ReifiedType)_specificFeature?

The interfaces are object types so it produces a shared generic and doesn't branch eliminate; have raised issue https://github.com/dotnet/coreclr/issues/16266 requesting ability to mark a method and have it be generated as non-shared (though may be a large Jit change).

If the method inlined (size aside), it would eliminate all the branches; but alas its an interface method and you can't inline through an interface.

Additionally an explicit interface method counts as virtual method https://github.com/dotnet/coreclr/issues/16198 so will only devirtualize on a sealed type where the explicit type is known/exposed at Jit time; so this can never devirtualize and inline (currently anyway):

((IFeatureCollection)this).Set<IHttpUpgradeFeature>(this);

Theoretically, isn't this exactly what Set<IHttpUpgradeFeature>(this) should optimize to?

To work around it you could define protected FastFeatureSet<TFeature>(TFeature instance) on HttpProtocol with an aggressive inline and hope it inlines into both two single feature resets (on Http1Connection and Http2Connection) and into the interface generic void IFeatureCollection.Set<TFeature>(TFeature instance), or not define it explicitly public Set<TFeature>(TFeature instance); else it introduces an extra method call and moves the potential interface inline one step further away; and crossgen isn't as happy with inlining shared generics with this number of type references (give error like "complex type handling"); the runtime Jit is happier to do it though.

It also seems a lot of work and hoops to jump through; as well as Jit beahviour to rely on just to avoid defining two simple Reset methods?

benaadams · 2018-02-17T06:25:39Z

halter73 requested changes

Were there other changes?

benaadams mentioned this pull request Feb 3, 2018

Faster IFeatureCollection.Get<TFeature> #2012

Closed

benaadams force-pushed the Faster-IFeatureCollection.Get branch from 5b345fa to 9c73a57 Compare February 10, 2018 23:21

benaadams force-pushed the Faster-IFeatureCollection.Get branch from 9c73a57 to d5d00e3 Compare February 10, 2018 23:27

Faster IFeatureCollection.Get<TFeature>

2d17398

benaadams force-pushed the Faster-IFeatureCollection.Get branch from cbd2130 to 2d17398 Compare February 11, 2018 00:59

muratg assigned halter73 Feb 13, 2018

muratg modified the milestones: 2.1.0-preview1, 2.1.0-preview2 Feb 13, 2018

halter73 suggested changes Feb 16, 2018

View reviewed changes

Add newline

dcb76de

halter73 approved these changes Feb 21, 2018

View reviewed changes

halter73 merged commit c0f88eb into aspnet:dev Feb 23, 2018

benaadams mentioned this pull request Feb 26, 2018

Use enum for method rather than string compares #2294

Merged

benaadams deleted the Faster-IFeatureCollection.Get branch March 2, 2018 03:28

benaadams mentioned this pull request Jan 31, 2020

[Jit] Proposal: Non-shared generics dotnet/runtime#9682

Open

halter73 mentioned this pull request Mar 24, 2020

Connection Abstractions dotnet/runtime#1793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster IFeatureCollection.Get<TFeature> #2290

Faster IFeatureCollection.Get<TFeature> #2290

benaadams commented Feb 3, 2018 •

edited

Loading

BrennanConroy commented Feb 3, 2018

benaadams commented Feb 10, 2018

benaadams commented Feb 10, 2018

davidfowl commented Feb 10, 2018

benaadams commented Feb 10, 2018

benaadams commented Feb 11, 2018

benaadams commented Feb 11, 2018 •

edited

Loading

benaadams commented Feb 11, 2018

halter73 left a comment

halter73 Feb 16, 2018

halter73 Feb 16, 2018

benaadams Feb 17, 2018

benaadams Feb 17, 2018

benaadams commented Feb 17, 2018

benaadams commented Feb 17, 2018

Faster IFeatureCollection.Get<TFeature> #2290

Faster IFeatureCollection.Get<TFeature> #2290

Conversation

benaadams commented Feb 3, 2018 • edited Loading

BrennanConroy commented Feb 3, 2018

benaadams commented Feb 10, 2018

benaadams commented Feb 10, 2018

davidfowl commented Feb 10, 2018

benaadams commented Feb 10, 2018

benaadams commented Feb 11, 2018

benaadams commented Feb 11, 2018 • edited Loading

benaadams commented Feb 11, 2018

halter73 left a comment

Choose a reason for hiding this comment

halter73 Feb 16, 2018

Choose a reason for hiding this comment

halter73 Feb 16, 2018

Choose a reason for hiding this comment

benaadams Feb 17, 2018

Choose a reason for hiding this comment

benaadams Feb 17, 2018

Choose a reason for hiding this comment

benaadams commented Feb 17, 2018

benaadams commented Feb 17, 2018

benaadams commented Feb 3, 2018 •

edited

Loading

benaadams commented Feb 11, 2018 •

edited

Loading